-
-
Notifications
You must be signed in to change notification settings - Fork 171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[16.0][MIG] dms #262
[16.0][MIG] dms #262
Conversation
versions of muk_dms than 12.0.2.0.0. Tested from 1.2.4 version.
In v13, this test is programmed in such a way that the demo user is supposed to be able to copy that subdirectory: https://github.com/OCA/dms/blob/c3f802db43362127e70d8c7b4987fb71d4c1f01c/dms/tests/test_directory.py#L40 However, in OCA#7 that test was modified indicating that demo user didn't have permissions to do that: https://github.com/OCA/dms/blob/e3b6d8d24534f2a68bfb88e310cc70cefe46bb64/dms/tests/test_directory.py#L39 Rolling back that change to ensure premissions remain the same in both versions of the module. Also changing the directory to test to ensure it contains no SVG files, whose detection seems to differ among environments, and which have some specific permission restrictions that can make the modified test fail or pass. @Tecnativa TT25645
This PR has the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are video and audio correctly previewed without the extra dependencies?
Please add #264 |
70ee0d4
to
162ff0c
Compare
162ff0c
to
05f7c67
Compare
With regards to the PR mentioned, the fix is in |
Well, I disagree here, as any other chance to have a non sudo access will provoke the same problem, so it's better to fix it in the root, as it also doesn't cause any cons. |
Well but the problem is that there is no path for reproduction, as @vancouver29 noted, and no test that can be written in There should be a comment explaining why it is useful, or at the very least this information should be in the commit message. Something like "while this field is not used, depending modules might need to search on |
OK, agreed of explaining everything better. The fix and its explanation can be added later in a subsequent PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me it looks very good now, i tested it and seems to be a nice module. Thanks for your effort :)
@pedrobaeza Can we merge this DMS PR? |
/ocabot merge nobump |
This PR looks fantastic, let's merge it! |
Congratulations, your PR was merged at f14e58c. Thanks a lot for contributing to OCA. ❤️ |
@pedrobaeza thank you very much for merging this work. But I don't see the changes in |
You didn't modify |
@vancouver29 You can check that your changes would be properly applied by running the maintainer tools manually. Just install it with pipx and run |
Now only having pre-commit, the readme regeneration is done. |
@pedrobaeza Sorry, my fault! Thanks for the hint again. I just created a new PR here #268
@len-foss, thank you very much for the hint. I will notice it next time :) |
[16.0][MIG] dms